fix: Cache middleware only caches static assets, not HTML pages#8
Conversation
The Cache middleware was setting max-age=604800 on ALL non-root responses, including HTML pages and 302 redirects. This caused Chrome to cache 302 redirect mappings persistently (this cache is NOT controlled by DevTools 'Disable cache'). When the system theme changed or different server instances had different themes, cached redirects in opposite directions created an infinite loop. Fix: Only set max-age=604800 for static assets (JS, CSS, fonts, images). HTML pages and other routes get no-cache to prevent stale redirects.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesCache control refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Cache middleware to target static assets for long-term caching using a new isStaticAsset helper function. The reviewer recommended optimizing this function by using a switch statement for better performance and converting extensions to lowercase to ensure case-insensitive matching.
| func isStaticAsset(path string) bool { | ||
| if path == "/" { | ||
| return false | ||
| } | ||
| staticExtensions := []string{".js", ".css", ".woff", ".woff2", ".ttf", ".eot", | ||
| ".svg", ".png", ".jpg", ".jpeg", ".gif", ".ico", ".webp", ".map", ".webmanifest"} | ||
| for _, ext := range staticExtensions { | ||
| if strings.HasSuffix(path, ext) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The current implementation of isStaticAsset performs a linear search through a slice on every request, which is inefficient ($O(N)$). Additionally, it is case-sensitive, meaning extensions like .JS or .CSS will not be recognized as static assets, leading to suboptimal caching for those files.
Using a switch statement on the file extension is more idiomatic in Go, provides better performance, and allows for easy case-insensitive matching.
func isStaticAsset(path string) bool {
dot := strings.LastIndexByte(path, '.')
if dot == -1 {
return false
}
ext := strings.ToLower(path[dot:])
switch ext {
case ".js", ".css", ".woff", ".woff2", ".ttf", ".eot", ".svg", ".png", ".jpg", ".jpeg", ".gif", ".ico", ".webp", ".map", ".webmanifest":
return true
}
return false
}1 participant
Problem
The Cache middleware was setting
max-age=604800(cache for one week) on ALL non-root responses, including HTML pages and 302 redirects.Chrome has a special in-memory cache for 302 redirect mappings that is NOT controlled by DevTools "Disable cache". When the system theme changed or different server instances had different themes, cached redirects in opposite directions created an infinite loop:
Fix
Only set
max-age=604800for static assets (JS, CSS, fonts, images, etc.). HTML pages and other routes getno-cacheto prevent stale redirects from being cached.This complements PR #7 which removed the server-side path redirect entirely.
Summary by CodeRabbit